Skoll Multi-Scan Review

Modes: review-securityOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
2 finding(s) not tied to a diff line (full detail below)

One or more scans did not complete — the results below are partial.

Posted findings

• [Medium] [review-security] SGX cpuid hard-set now forces CPUID_VAES | CPUID_AVX512, can crash AES-GCM on SGX CPUs lacking those extensions — wolfcrypt/src/cpuid.c:56-57
• [Medium] [review-security] New Thumb-2 AES-GCM-SIV asm and C wrappers are neither built nor run in the added CI — .github/workflows/aesgcm-siv.yml:60-148

Findings not tied to a diff line

POLYVAL hash-key stack temporaries not zeroized (t, hrev)

File: wolfcrypt/src/aes.c (AesGcmSivPolyvalInit asm branch; AesGcmSivPolyvalInitSw)
Function: AesGcmSivPolyvalInit / AesGcmSivPolyvalInitSw
Severity: Low
Category: Zeroization

The per-message POLYVAL key is derived from the secret message-authentication-key. The persistent copies in AesGcmSivPolyval (poly->h / poly->m / poly->hHw) are correctly wiped by AesGcmSivPolyvalFinal via ForceZero(poly, sizeof(*poly)). However the intermediate stack buffers that hold transformed copies of that key are not wiped before going out of scope: byte t[WC_AES_BLOCK_SIZE] in the asm key-prep branch of AesGcmSivPolyvalInit (holds ByteReverse(mulX(ByteReverse(h)))), and byte hrev[WC_AES_BLOCK_SIZE] in both AesGcmSivPolyvalInitSw variants (holds ByteReverse(h)). These leave a permutation of the POLYVAL key on the stack after the function returns. This is defense-in-depth only (recovering it would let an attacker forge for that specific key+nonce), and the buffers are typically overwritten by later stack frames.

Recommendation: Add ForceZero(t, sizeof(t)); before the asm key-prep block returns and ForceZero(hrev, sizeof(hrev)); at the end of each AesGcmSivPolyvalInitSw variant, consistent with the zeroization used elsewhere in the SIV code.

Referenced code: wolfcrypt/src/aes.c (AesGcmSivPolyvalInit asm branch; AesGcmSivPolyvalInitSw) (9 lines)

ARM64 GHASH length conversion changed from ubfiz (#3,#32) to lsl #3, dropping upper-32-bit masking

File: wolfcrypt/src/port/arm/armv8-aes-asm.S (multiple *_partial_done labels); wolfcrypt/src/port/arm/armv8-aes-asm_c.c (AES_GCM_encrypt_final/decrypt_final/_AARCH64 paths)
Function: AES_GCM_*_AARCH64 / aes_gcm_*_arm64_crypto_*_partial_done
Severity: Info
Category: Logic

In the existing (non-SIV) AES-GCM AArch64 code regenerated by this PR, byte-length-to-bit-length conversions changed from ubfiz xN, xN, #3, #32 to lsl xN, xN, #3, and standalone ubfiz xN, xN, #0, #32 truncations (e.g. on tagSz in the *_part_tag paths) were removed. ubfiz #3,#32 masks the operand to its low 32 bits before shifting; lsl #3 shifts the full 64-bit register. The two are identical for any operand whose upper 32 bits are zero, which is always the case for the word32 length/tag-size arguments as materialized by wolfSSL callers (32-bit values are zero-extended). So no wrong tag results for valid wolfSSL usage, and these GCM paths are covered by the broader GCM test suite. The note is only that the new sequence is marginally less defensive against a caller leaving non-zero garbage in the upper 32 bits of the argument register.

Recommendation: No action required if these are deliberate asm-generator output; the behavior is equivalent for all in-tree callers. If the generator can emit it cheaply, retaining a 32-bit mask (or documenting that callers must pass zero-extended lengths) preserves the prior defensiveness.

Referenced code: wolfcrypt/src/port/arm/armv8-aes-asm.S (multiple *_partial_done labels); wolfcrypt/src/port/arm/armv8-aes-asm_c.c (AES_GCM_encrypt_final/decrypt_final/_AARCH64 paths) (4 lines)

Review generated by Skoll

macOS/clang build break:

--enable-intelasm --enable-aesgcm-siv fails to compile on an Intel MacBookPro:

aes.c:885: error: unused function 'AesEcbEncryptBlocks' [-Werror,-Wunused-function]
aes.c:912: error: unused function 'AesEcbDecryptBlocks'
aes.c:997: error: unused function 'AesCtrEncryptBlocks'

These three new static helpers' definitions are compiled in the AES-NI config, but their call sites (lines 7796/15153/15252) are behind narrower #if guards, so without AVX512/AES-ECB they're defined-but-unused. gcc tolerates unused static inline; clang -Werror doesn't, which is why all the Linux x86 hosts passed and only the Mac caught it.

Fix: align the guards (only define each helper when a caller is compiled) or mark them unused.